Skip to content

Conversation

@iamgabrielma
Copy link
Contributor

@iamgabrielma iamgabrielma commented Aug 21, 2025

Closes WOOMOB-125

Description

This PR adds a functional two-side navigation for POS settings. UI is not final, just temporary to build the settings scaffolding and navigation. UI details will be handled separately.

  • Settings is full screen. Presented via ... > Settings
  • Left side contains the parent navigation: Store, Hardware (top), and Help (pinned to the bottom).
  • Right side contains the detail navigation view. Further navigation will be encapsulated to the right-side. At the moment only Hardware has been added with barcode scanner, and card readers sub-navigation. The other views will present just a placeholder.
  • Can be dismissed to go back to POS dashboard.

Testing information

  • Switch pointOfSaleSettingsi1 to true
  • Navigate to POS, tap on the floating ..., tap on Settings
  • Observe the view is presented full-screen
  • Can be dismissed by tapping "Done"
  • Tapping any of the right side items displays a custom view to the left
  • Tap Hardware, observe there are two more items to navigate to: Card readers and barcode scanner
  • Observe how you can navigate to those as well.

Screenshots

Screen.Recording.2025-08-22.at.16.05.17.mov

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

It doesn’t seem to be straight-forward to push one of the elements of a List down to the bottom, while keeping the rest at the top, so “Help” has to be outside the List.
We also remove the navigation stack from the settings view as a parent, otherwise is part of the nav stack, and presenting any other destination would cover it full-screen. We only make the details right-side part of the nav stack instead.
Dismissal does not show anymore in the parent settings view since we don’t us a navigation stack, and we cannot use a navigation stack otherwise the destinations will be presented full-screen, so we move dismissal to each subview.
@dangermattic
Copy link
Collaborator

dangermattic commented Aug 21, 2025

1 Warning
⚠️ This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

}
}
}
.safeAreaInset(edge: .bottom) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't able to pin the Help section to the bottom of the view while this is part of a List. This approach worked, but I'm not fully convinced is the best way to move forward.

We do not necessarily need a list here and perhaps creating just a reusable "card" would work fine for the 3 items. If we do, this can be iterated on a separate PR. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, if a list makes it harder, let's just make our own cards.

Also, this might move the help button above an on-screen keyboard, which would be a bit strange. It's likely that we'll end up presenting one in settings eventually.

.foregroundStyle(.secondary)
}
.padding()
.toolbar {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a NavigationStack gotcha: I had to add the .toolbar for the dismiss action to all destination views, rather than once to the parent Settings view, if we do, then these destinations will be presented full-screen as part of the stack, rather than encapsulated to the right-side only.

Copy link
Contributor Author

@iamgabrielma iamgabrielma Aug 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative that seems to work is to dismiss the PointOfSaleSettingsView through a viewmodifier attached to the parent view instead, so we could remove the .toolbar approach from the subviews. For testing I used .posModalCloseButton(action: { dismiss() }) since already exists, and seemed to work fine. no longer needed since using a POSPageHeaderView: 86fe351

@iamgabrielma iamgabrielma added type: task An internally driven task. feature: app settings Related to settings accessed via the gear icon in the My Store section. feature: POS labels Aug 21, 2025
@iamgabrielma iamgabrielma added this to the 23.2 milestone Aug 21, 2025
@iamgabrielma iamgabrielma marked this pull request as ready for review August 21, 2025 09:31
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Aug 21, 2025

App Icon📲 You can test the changes from this Pull Request in WooCommerce iOS Prototype by scanning the QR code below to install the corresponding build.

App NameWooCommerce iOS Prototype
Build Numberpr16021-78b3836
Version23.1
Bundle IDcom.automattic.alpha.woocommerce
Commit78b3836
Installation URL1iac8mtkgishg
Automatticians: You can use our internal self-serve MC tool to give yourself access to those builds if needed.

@iamgabrielma iamgabrielma requested a review from joshheald August 21, 2025 10:08
Comment on lines 9 to 12
Text(Localization.navigationTitle)
.font(.posHeadingRegular)
.padding(.horizontal, POSPadding.medium)
.padding(.top, POSPadding.medium)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Povilas shared his experiments with historic orders view in slack and had a good suggestion for when we update the UI: This bit can be updated with a POSPageHeaderView across the top of the parent view

Screenshot 2025-08-22 at 15 54 40

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated on 86fe351

@joshheald joshheald self-assigned this Aug 26, 2025
Copy link
Contributor

@joshheald joshheald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions inline.

Works as described, fine to merge behind the feature flag.

Did you try using split views? My concern with the HStack approach is that it can be harder to manage at smaller screen sizes when we want to collapse it. With the coming iOS 26 having resizable windows, it might be best to take that pain up front and get it working with a split view instead of an HStack...

Comment on lines +8 to +16
POSPageHeaderView(
title: Localization.navigationTitle,
trailingContent: {
Button(action: { dismiss() }) {
Text(Image(systemName: "xmark"))
.font(.posButtonSymbolLarge)
}
.foregroundColor(.posOnSurface)
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to have this view handle back navigation for child views – the in-line navigation bar is a bit rough.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in the first draft of designs that we've just been provided with, the Settings do not seem to have any header, so we may get rid of this bit entirely and leave navigation to the child/side view entirely. I'll iterate on this shortly 👍

}
}
}
.safeAreaInset(edge: .bottom) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, if a list makes it harder, let's just make our own cards.

Also, this might move the help button above an on-screen keyboard, which would be a bit strange. It's likely that we'll end up presenting one in settings eventually.

Comment on lines +60 to +63
.background(
RoundedRectangle(cornerRadius: POSCornerRadiusStyle.large.value, style: .continuous)
.fill(selection == .help ? Color.accentColor : Color.clear)
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The selected version looks pretty odd, being purple while the other selected items go grey.

Having rounded corners doesn't work well with the padding we have here either – which is closer to the screen edges because it's outside the list, but if we keep it this way then square corners would make more sense.

I know UI's not final so no need to fix here, but thought I'd mention it.

}

private extension PointOfSaleSettingsView {
enum SidebarNavigation: String, CaseIterable, Identifiable {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be nicer to define a protocol for settings items instead, then each can be separately defined closer to their content? Not sure about this, just something to consider.

Comment on lines +83 to +117
extension PointOfSaleSettingsView {
enum HardwareDestination: Identifiable, CaseIterable {
case cardReaders
case scanners

var id: Self { self }

var title: String {
switch self {
case .cardReaders:
return Localization.hardwareNavigationCardReaderTitle
case .scanners:
return Localization.hardwareNavigationBarcodeTitle
}
}

var subtitle: String {
switch self {
case .cardReaders:
return Localization.hardwareNavigationCardReaderSubtitle
case .scanners:
return Localization.hardwareNavigationBarcodeSubtitle
}
}

var icon: String {
switch self {
case .cardReaders:
return "creditcard"
case .scanners:
return "qrcode.viewfinder"
}
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like overreach for this to be defined in the settings view. Surely this would be better in PointOfSaleSettingsHardwareDetailView?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True that, I'll address this one along #16038 , as PointOfSaleSettingsView.HardwareDestination changes 👍

Comment on lines +17 to +19
HStack(spacing: POSSpacing.none) {
VStack(alignment: .leading, spacing: POSSpacing.none) {
List(selection: $selection) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider breaking these up into subviews... aim for something clearer at the top level like:

var body: some View {
    POSPageHeaderView(
        ...
    )
    HStack {
        PointOfSaleSettingsList()
        PointOfSaleSettingsDetail()
    }
}

You'll need to pass some bindings around but it'll be both easier to read/debug in future, and should perform better (not that there's a performance issue!)

}
.foregroundColor(.posOnSurface)
})
HStack(spacing: POSSpacing.none) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this should be 50/50 split...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is definitely not 😂 , already addressed on #16038

@iamgabrielma iamgabrielma merged commit f1267a9 into trunk Aug 27, 2025
14 checks passed
@iamgabrielma iamgabrielma deleted the task/WOOMOB-1025-two-panel-settings-view branch August 27, 2025 00:38
@iamgabrielma
Copy link
Contributor Author

Sorry I missed this comment!

Did you try using split views? My concern with the HStack approach is that it can be harder to manage at smaller screen sizes when we want to collapse it. With the coming iOS 26 having resizable windows, it might be best to take that pain up front and get it working with a split view instead of an HStack...

Sort of, it's useable with a split view but it's not the best:

Simulator Screenshot - iPad mini (A17 Pro) - US store - 2025-08-28 at 08 15 17

In theory that's an unsupported width, but we do show the floating menu and merchants can navigate through the rest of options, my plan is to use a similar approach of what POS Orders is doing and to show the side menu only as full screen, then navigate to the details view as full screen.

Simulator Screenshot - iPad mini (A17 Pro) - US store - 2025-08-28 at 08 15 06

I'm delaying it a bit to see if the design proposal for next steps changes the overall design too much or not (we should know it today/tomorrow).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature: app settings Related to settings accessed via the gear icon in the My Store section. feature: POS type: task An internally driven task.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants